Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize writes across all instances of ConsoleSink #65

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

nblumhardt
Copy link
Member

Currently, each instance of ConsoleSink is synchronized so that concurrent output is not interleaved.

Some scenarios, like reinitialization of the logging pipeline, non-singleton loggers, and Serilog.Sinks.Map can result in multiple instances of ConsoleSink being constructed, and today these will happily interleave output. (E.g. see serilog/serilog#1350 .)

This PR switches the lock from sink-specific to shared, so that at least all writes through console sinks will be non-interleaved (even if other console output isn't controlled this way).

CC @skomis-mm

@adamchester
Copy link
Member

I made a different decision, after chatting with @nblumhardt we thought it might be useful, but still a little undecided.

So I pushed it and made pr #66. Probably we can consider that change as a breaking change for the next major version increment.

@nblumhardt nblumhardt merged commit 7737b6d into serilog:dev Aug 2, 2019
@nblumhardt nblumhardt mentioned this pull request Jul 12, 2021
@oising
Copy link

oising commented Sep 1, 2021

Is this only a "breaking" change because the output is no longer interleaved, or is it genuinely incompatible with a component expecting 3.1.1 (for example?)

@nblumhardt
Copy link
Member Author

@oising the binary signature of WriteTo.Console() will change to include the syncRoot argument. This won't break source code, since the argument has a default value (null), but any other binaries compiled against the old version of Console() will need recompilation. HTH!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants